Core, API: Append to branch Impl by namrathamyske · Pull Request #5010 · apache/iceberg

namrathamyske · 2022-06-09T23:02:46Z

Issue addressed from: #3896

This PR contains the Implementation part from #4926 for only Append to Table operation.

Changes addressed:

Code to update to a branch.
Fetching the parent snapshot in the branch so as to make changes only to the specific branch.
Similarly fetching manifests from parent snapshot in a specific branch.

Append changes to snapshot branch - changes after review comments remove space Adding support methods for supporting performing append/delete on branch Removing spaces Removing spaces Removing unwanted vars Adding check for null and invalid branch Adding check for null and invalid branch

namrathamyske · 2022-06-09T23:08:58Z

@amogh-jahagirdar @rdblue Here are the implementation changes as addressed in #4926 (comment)

namrathamyske · 2022-06-13T16:28:48Z

@jackye1995 Is it possible for you to review this?

cc: @SreeramGarlapati

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

namrathamyske · 2022-06-14T19:19:08Z

@amogh-jahagirdar incorporated your feedback. Thanks for your review.

namrathamyske · 2022-06-20T17:21:38Z

@rdblue Can you review this ?

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

core/src/test/java/org/apache/iceberg/TestFastAppend.java

namrathamyske · 2022-06-21T17:47:27Z

@amogh-jahagirdar Thanks for your review ! Incorporated the changes requested.

checkStyle errors

…o-branch

namrathamyske · 2022-07-09T18:54:28Z

@rdblue @amogh-jahagirdar I have implemented toBranch method only for Append operations as per suggestion in #4926 (comment). For other operations it will throw an UnsupportedOperationException. Please let me know if this looks good? Can be merged only after #4926

fixing tests add new ref Removing unncessary methods spell checks spell checks

namrathamyske · 2022-07-10T20:07:00Z

@rdblue @amogh-jahagirdar After appending to a branch, The sequence number for main ref is continued from last number from branch ref. Should the sequence number continue from last last commit in main ref or branch ref ? Any thoughts appreciated.

If the sequence number is continued from branch ref, the linearity is broken. But if we continue from main ref, we will have snapshots with same sequence number.

dimas-b · 2022-07-11T20:39:39Z

core/src/main/java/org/apache/iceberg/SnapshotProducer.java


+  @Override
+  public ThisT toBranch(String branch) {
+    throw new UnsupportedOperationException("Performing operations on a branch is currently not supported");


nit: the error wording looks a bit confusing to me. Are we planning to support the toBranch method in this class later or is it not envisioned to be supported on this particular class at all? thx!

@dimas-b , In the PR i have implemented the toBranch only in one of the subclasses of Snapshot producer aka FastAppend class. This is done in order to avoid creating a huge PR and be thorough in testing each of the child operations of snapshot producer. In future my aim is to incrementally raise PR for each of them.

Thanks for the clarification, @namrathamyske ! Would you mind updating the message to something like Performing SnapshotProducer operations on a branch is currently not supported to avoid making a (false) impression that it is not supported in "general"?.. or use different wording as you see fit. My only concern is that getting the old message in one context might be interpreted that it is not supported anywhere.

rdblue · 2022-07-31T22:44:45Z

core/src/main/java/org/apache/iceberg/FastAppend.java


+  @Override
+  public FastAppend toBranch(String branch) {
+    Preconditions.checkArgument(branch != null, "branch cannot be null");


Message should be Invalid branch name: null

rdblue · 2022-07-31T22:45:22Z

core/src/main/java/org/apache/iceberg/FastAppend.java

+  public FastAppend toBranch(String branch) {
+    Preconditions.checkArgument(branch != null, "branch cannot be null");
+    if (ops.current().ref(branch) == null) {
+      super.createNewRef(branch);


This should be handled by SnapshotProducer, as I noted on #4926.

rdblue · 2022-07-31T22:45:53Z

core/src/main/java/org/apache/iceberg/FastAppend.java

+
+    Preconditions.checkArgument(ops.current()
+            .ref(branch).type()
+            .equals(SnapshotRefType.BRANCH),


Does this check need to be split across 3 lines? Or is that autoformatting doing it?

amogh-jahagirdar · 2022-08-03T02:46:19Z

core/src/main/java/org/apache/iceberg/FastAppend.java

+    Preconditions.checkArgument(ops.current()
+            .ref(branch).type()
+            .equals(SnapshotRefType.BRANCH),
+        "%s is not a ref to type branch", branch);


Also now that we're creating a branch if it doesn't exist, on commit, we should do a null check. It's okay if the branch is null, since we'll create it. if it's not null, it must be an actual branch. I think this should also reside in snapshot producer as well.

nkeshavaprakash added 2 commits June 9, 2022 13:22

Adding snapshot to branch Impl

e6ea5f6

github-actions bot added API core labels Jun 9, 2022

namrathamyske changed the title ~~Append changes to branch Impl (contd from https://github.com/apache/iceberg/pull/4926)~~ Append changes to branch Impl Jun 9, 2022

namrathamyske changed the title ~~Append changes to branch Impl~~ Core, API: Append changes to branch Impl Jun 9, 2022

amogh-jahagirdar reviewed Jun 13, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/SnapshotProducer.java Outdated Show resolved Hide resolved

amogh-jahagirdar reviewed Jun 13, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/SnapshotProducer.java Outdated Show resolved Hide resolved

Adding check to see if it's a branch

b43b80b

nkeshavaprakash added 2 commits June 16, 2022 13:44

Creating branch if ref absent

9cb39c2

Building after setting ref

ba341ec

namrathamyske mentioned this pull request Jun 20, 2022

Support Iceberg branching for all the snapshot producer operations #3896

Closed

namrathamyske changed the title ~~Core, API: Append changes to branch Impl~~ Core, API: Append/Delete changes to branch Impl Jun 20, 2022

amogh-jahagirdar reviewed Jun 21, 2022

View reviewed changes

Setting default branch as main

d1ca432

nkeshavaprakash added 5 commits June 22, 2022 14:04

Fetching current when no main ref is set.

0d79ef0

checkStyle errors

0e2f690

checkStyle errors

Merge branch 'master' of https://github.com/apache/iceberg

bcfb7a8

checkStyle fixes

495c0f3

build fixes

f7464eb

namrathamyske mentioned this pull request Jul 2, 2022

Core, API: RowDelta changes to branch Impl #5181

Closed

nkeshavaprakash added 4 commits July 5, 2022 22:55

Merge branch 'master' of https://github.com/apache/iceberg

8d80888

Merge branch 'master' of https://github.com/apache/iceberg into add-t…

ca774b2

…o-branch

Restricting toBranch only for append

fba4643

Restricting toBranch only for append

5456a3a

namrathamyske changed the title ~~Core, API: Append/Delete changes to branch Impl~~ Core, API: Append to branch Impl Jul 9, 2022

checkStyle fixes

7ecc325

namrathamyske mentioned this pull request Jul 9, 2022

Core, API: Performing operations on a snapshot branch ref #4926

Merged

namrathamyske marked this pull request as draft July 10, 2022 00:14

adding setters and getters for targetBranch

637b2e8

fixing tests add new ref Removing unncessary methods spell checks spell checks

namrathamyske mentioned this pull request Jul 10, 2022

Core, API: BaseRowDelta, BaseOverwrite,BaseReplacePartitions, BaseRewrite to branch Impl #5234

Merged

nkeshavaprakash added 2 commits July 10, 2022 13:21

Adding more checks in testcase

40e5e62

indentation fixes

13a8e73

dimas-b reviewed Jul 11, 2022

View reviewed changes

rdblue reviewed Jul 31, 2022

View reviewed changes

amogh-jahagirdar reviewed Aug 3, 2022

View reviewed changes

namrathamyske closed this Aug 29, 2022

Conversation

namrathamyske commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namrathamyske commented Jun 9, 2022

Uh oh!

namrathamyske commented Jun 13, 2022

Uh oh!

Uh oh!

Uh oh!

namrathamyske commented Jun 14, 2022

Uh oh!

namrathamyske commented Jun 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

namrathamyske commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namrathamyske commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namrathamyske commented Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

namrathamyske Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

dimas-b Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue Jul 31, 2022

Choose a reason for hiding this comment

Uh oh!

rdblue Jul 31, 2022

Choose a reason for hiding this comment

Uh oh!

rdblue Jul 31, 2022

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

namrathamyske commented Jun 9, 2022 •

edited

Loading

namrathamyske commented Jun 21, 2022 •

edited

Loading

namrathamyske commented Jul 9, 2022 •

edited

Loading

namrathamyske commented Jul 10, 2022 •

edited

Loading

dimas-b Jul 11, 2022 •

edited

Loading

amogh-jahagirdar Aug 3, 2022 •

edited

Loading